-
Notifications
You must be signed in to change notification settings - Fork 11
migrate hyper to reqwest #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@adnanademovic: it'd be great if you could review, merge & release this and then bump this is also a good reason to not combine that release with switching to |
`hyper` v0.10 is extremely outdated, the current release is v1.6. this old version pulls in vulnerable dependencies (incl. `hyper` v0.10 itself). rather than upgrading to `hyper` v1.6 i opted to replace it with `reqwest` since this crate here is not `async` and `hyper` v1 is completely `async`. due to the very limited amount of features needed from `hyper` (just executing HTTP POST requests) it can instead be replaced with the much simpler `reqwest` crate. while it hasn't hit v1.0 yet it is very widely used (nearly 200 million downloads on crates.io). this is a breaking change for consumers since the URLs can now just be passed as strings rather than having to call `.parse()?` on the string. note that this is similar to, but not the same as, adnanademovic#8. fixes adnanademovic#9
code changes done using `cargo fix --edition`
215825c
to
eac04e6
Compare
this updates to the latest `xml-rpc` version. note that at the moment this does not yet build since it hasn't been released yet. this requires adnanademovic/xml-rpc-rs#11 to be merged & released first. for the time being you can use this by using `[patch.crates-io]` in your `Cargo.toml` and overwrite both `rosrust` and `xml-rpc`. this update is needed to resolve various security vulnerabilities coming from outdated versions of `hyper` which are being pulled in via `xml-rpc`. with this, `cargo-audit` is happy again.
i noticed a problem with this several layers further up: when used within an async context (as e.g. in this
the suggestion works, but it does mean that users of thus, a call like this: rosrust::loop_init("talker_listener", 1000); becomes: tokio::task::spawn_blocking(|| {
rosrust::loop_init("talker_listener", 1000);
})
.await?; in the linked i do believe that this is still the correct thing to do ( |
please see the individual commit messages for further details.
i realise that this is similar to #8 from @jakobbraun, however this PR completely removes
hyper
and it does not add anasync
version of the API. i think removing the outdatedhyper
dependency (and the vulnerabilities coming from it) is the most pressing issue.note that this is a breaking change for consumers, so you might want to use that chance to just ditch the blocking API (currently the only one available) and replace it with an
async
API (most of the rust ecosystem - at the very least anything doing I/O - seems to have gone down that route, and for good reasons). i've raised #12 for this.nevertheless i think that merging & releasing this PR would be a good first step, you can then just remove the
blocking::
from thereqwest
module name and add the necessaryasync
/await
keywords. having a last non-async
release without the outdatedhyper
dependency would allow clients which are not (yet)async
to at least get rid of the security warnings coming from the outdatedhyper
dependency, giving them a two-step migration path (first get rid of insecure dependencies and then switch toasync
).fixes #9